-
Notifications
You must be signed in to change notification settings - Fork 576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Flexible Sync #5301
Support Flexible Sync #5301
Conversation
//////////////////////////////////////////////////////////////////////////// | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file does not need to be reviewed. The content is merely moved from Configuration.ts
into this /schema/validate.ts
file.
RealmEventName, | ||
RealmListenerCallback, | ||
SubscriptionOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kraenhansen Do you know why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I have a few general questions just to broaden my understanding of things (could be answered by other reviewers). Other than that, I would like my this.timeout
issue to be addressed and this is good to go in my eyes 👍🏼
const { path, schema, onMigration, sync } = config; | ||
if (typeof onMigration !== "undefined") { | ||
assert.function(onMigration, "migration"); | ||
assert.object(config, "realm configuration", { allowArrays: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense. What's holding that back?
@@ -593,11 +630,11 @@ export class Realm { | |||
this.syncSession?.resetInternal(); | |||
} | |||
|
|||
// TODO: Support embedded objects and asymmetric sync | |||
// TODO: Support embedded objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general question, but do we have a task to track this? I haven't heard embedded object support mentioned in any of our planning meetings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we already supported them. If not, I would expect (hope!) that we would have some failing tests related to that.
@@ -75,6 +75,10 @@ export class Results<T = unknown> extends OrderedCollection<T> { | |||
return this.internal.size(); | |||
} | |||
|
|||
description(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: is the description
the filtered
string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is core's representation of the query. Some SDKs are not using RQL so it is a serialization of the query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the comments have been resolved, it is ready to be merged.
It is a very big PR, and it has many moving parts. I am happy to see that you have been able to pull it through.
@@ -45,24 +45,41 @@ describe.skipIf(environment.missingServer, "Asymmetric sync", function () { | |||
}, | |||
}); | |||
|
|||
it("Schema with asymmetric = true and embedded = false", function () { | |||
it("Schema with asymmetric = true and embedded = false", function (this: RealmContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(General comment, not specific to this line)
Should any of these changes be backported to the master branch or are they bindgen-specific? Ideally, we want the tests in bindgen and master to be as in-sync as possible since that is how we are ensuring that we don't change behavior (except where we intend to!)
/** | ||
* Validate the fields of a user-provided realm sync configuration. | ||
*/ | ||
export function validateSyncConfiguration(config: unknown): asserts config is SyncConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure if it is a good idea to split parsing and validation like this. It seems easy for them to get out of sync. Not sure if you want to change this now, but something to consider adding to a todo list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a note of this 👍 Also good to be discussed in the team.
Co-authored-by: Kræn Hansen <[email protected]>
Co-authored-by: Kræn Hansen <[email protected]>
What, How & Why?
Adds support for Flexible Sync and Asymmetric Sync.
Flexible Sync:
flexible: true
and optionally aninitialSubscriptions
object:realm.subscriptions
Asymmetric Sync:
asymmetric: true
in schema of a Realm object. If set, then:Realm.create()
will returnundefined
rather than the object itself.Review Notes:
packages/realm/src/schema/validate.ts
does not need to be reviewed. The content here has merely been moved fromConfiguration.ts
.API change:
{ sync?: SyncConfiguration | true }
wheresync
must betrue
.{ sync?: SyncConfiguration; openSyncedRealmLocally?: true }
Will be added in a separate PR:
BaseSubscriptionSet
such asjoin
,filter
,every
,concat
, etc. so that it can extendReadonlyArray<Subscription>
.This closes #4806 and #5270.
☑️ ToDos